Skip to content

bugfix/CSTACK-154: Deletion of iscsi storage pool should not try to d…#47

Closed
piyush5netapp wants to merge 4 commits intomainfrom
bugfix/CSTACKEX-154
Closed

bugfix/CSTACK-154: Deletion of iscsi storage pool should not try to d…#47
piyush5netapp wants to merge 4 commits intomainfrom
bugfix/CSTACKEX-154

Conversation

@piyush5netapp
Copy link
Copy Markdown

@piyush5netapp piyush5netapp commented Apr 21, 2026

…elete igroup

Description

This PR...

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • Build/CI
  • Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

Delete igroup call was not made to ontap . Checked through logs.

How did you try to break this feature and the system with this change?

@github-actions
Copy link
Copy Markdown

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.


try {
OntapResponse<Igroup> igroupResponse = sanFeignClient.getIgroupResponse(authHeader, igroupParams);
if (igroupResponse == null || igroupResponse.getRecords() == null || igroupResponse.getRecords().isEmpty()) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (igroupResponse == null || igroupResponse.getRecords() == null || igroupResponse.getRecords().isEmpty()) {
if (igroupResponse == null || StringUtils.isBlank(igroupResponse.getRecords())) {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This we need to do for all files across ontap foler. Can be taken up as code refactoring task.

Copy link
Copy Markdown
Collaborator

@sandeeplocharla sandeeplocharla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall it looks good

Copy link
Copy Markdown

@rajiv-jain-netapp rajiv-jain-netapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s add a clarification here: the workflow should not attempt to delete SAN igroups during storage‑pool deletion. Our current approach relies on ONTAP automatically cleaning up the igroups during the final unmap operation, which should be sufficient and avoids redundant handling in the workflow.
Additionally, addressing Sandeep’s observation — while it is correct that a storage pool cannot be created without hosts initially, we should consider a downstream scenario. For example, if a storage pool is successfully created and, at a later point (day‑N), the associated hosts become unreachable or are removed, do we end up retaining a stale storage‑pool entry in CloudStack?
This is something we can analyze and validate separately.

@github-actions
Copy link
Copy Markdown

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

@piyush5netapp
Copy link
Copy Markdown
Author

Let’s add a clarification here: the workflow should not attempt to delete SAN igroups during storage‑pool deletion. Our current approach relies on ONTAP automatically cleaning up the igroups during the final unmap operation, which should be sufficient and avoids redundant handling in the workflow. Additionally, addressing Sandeep’s observation — while it is correct that a storage pool cannot be created without hosts initially, we should consider a downstream scenario. For example, if a storage pool is successfully created and, at a later point (day‑N), the associated hosts become unreachable or are removed, do we end up retaining a stale storage‑pool entry in CloudStack? This is something we can analyze and validate separately.

As said above replying Sandeep's comment, we have decided to not fail storage pool creation even if there is not host or host is not in appropriate state. This type of behaviour is followed by majority of storage vendor.Also, while creating igroup , we are setting delelte_on_unmap=true . This also answers the downstream scenario.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants